Pass match_water parameter to specialised solvation functions #190
Conversation
lohedges
added
bug
lohedges
temporarily deployed
to
biosimspace-build
GitHub Actions
Inactive
lohedges
temporarily deployed
to
biosimspace-build
GitHub Actions
Inactive
lohedges
temporarily deployed
to
biosimspace-build
GitHub Actions
Inactive
lohedges
temporarily deployed
to
biosimspace-build
GitHub Actions
Inactive
lohedges
temporarily deployed
to
biosimspace-build
GitHub Actions
Inactive
There was a problem hiding this comment.
Looks good - Am I right in remembering that the root cause of the bug was the change in the number of positional arguments, so match_water wasn't passed correctly? Is it worth switching to keyword arguments for these many-argument functions?
(no need to do this for this PR fix - but something to consider for the future)
[ignore - posted this because I misread a GH notification - I thought this PR was familiar!]
|
Yes, that's right. They should really be keywords. They are everywhere other than the validation function, so I'll update that in future. There used to only be a few, but it makes sense to switch to keywords now they have proliferated. No worries. I forgot to remove the review request since it was identical to the previous PR. |
This PR fixes a bug where the
match_waterparameter wasn't passed from the genericBioSimSpace.Solvent.solvatefunction through to the specialised functions for the supported water models. I've generalised the test so it uses both function types so that we catch similarly missed parameters in future, e.g. when anything else is added.Closes #185.
develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]Suggested reviewers:
@chryswoods